Describe LinearOperator{T, S}(args...) in docs#405
Conversation
Add documentation for JuliaSmoothOptimizers#383 Also cleans up an unnecessarily large number of `LinearOperator(M)` methods, preferring to consolidate them by supplying `M`-specific defaults for the `symmetric` and `hermitian` keyword arguments.
There was a problem hiding this comment.
Pull request overview
This PR adds documentation for the type-inferrable constructor variant LinearOperator{T, S}(args...) following PR #383, which moved the storage type parameter S from a keyword argument to a type parameter for better type inference. The PR also refactors the matrix-based constructors by consolidating multiple specialized methods (for Symmetric, Hermitian, and SymTridiagonal) into helper functions that provide type-specific defaults for the symmetric and hermitian properties.
Changes:
- Added documentation for
LinearOperator{T, S}(nrow, ncol, ...)constructor with tips recommending its use in performance-sensitive applications - Refactored matrix constructors to use
defaultsymmetric(M)anddefaulthermitian(M)helper functions instead of multiple specialized methods - Added default parameter
ctprod! = nothingtoLinearOperator{T, S}(nrow, ncol, ...)for consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/constructors.jl | Consolidated matrix constructors using helper functions, added tips about using LinearOperator{T, S} for performance, and updated documentation |
| src/abstract.jl | Added documentation for LinearOperator{T, S}(nrow, ncol, ...) constructor and added default ctprod! = nothing parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end | ||
| defaultsymmetric(M) = false | ||
| defaulthermitian(M) = false | ||
| defaultsymmetric(M::Symmetric) = true |
There was a problem hiding this comment.
This refactoring introduces a subtle behavioral change for complex Symmetric matrices. Previously, only real Symmetric matrices matched a specialized constructor (setting symmetric=true, hermitian=true), while complex Symmetric matrices fell back to the default constructor with symmetric=false, hermitian=false. Now, the defaultsymmetric(M::Symmetric) = true method applies to all Symmetric matrices, including complex ones. This is mathematically correct (a symmetric matrix is symmetric regardless of element type), but it is a breaking change that should be documented in release notes.
| defaultsymmetric(M::Symmetric) = true | |
| defaultsymmetric(M::Symmetric{<:Real}) = true |
There was a problem hiding this comment.
I think it would be fair to call this a bugfix, not a breaking change.
| LinearOperator(M::AbstractMatrix{T}; symmetric=defaultsymmetric(M), hermitian=defaulthermitian(M), S = Vector{T}) where {T} | ||
| Construct a linear operator from a dense or sparse matrix. Use the optional | ||
| keyword arguments to indicate whether the operator is symmetric and/or | ||
| hermitian. Change `S` to use LinearOperators on GPU. |
There was a problem hiding this comment.
The documented default S = Vector{T} is imprecise. The actual default is S = storage_type(M) (line 17), which typically evaluates to Vector{T} for most matrices but can differ for specific types like Diagonal. For accuracy, the documentation should either state S = storage_type(M) or clarify that the default storage type is determined by storage_type(M).
| LinearOperator(M::AbstractMatrix{T}; symmetric=defaultsymmetric(M), hermitian=defaulthermitian(M), S = Vector{T}) where {T} | |
| Construct a linear operator from a dense or sparse matrix. Use the optional | |
| keyword arguments to indicate whether the operator is symmetric and/or | |
| hermitian. Change `S` to use LinearOperators on GPU. | |
| LinearOperator(M::AbstractMatrix{T}; symmetric=defaultsymmetric(M), hermitian=defaulthermitian(M), S = storage_type(M)) where {T} | |
| Construct a linear operator from a dense or sparse matrix. Use the optional | |
| keyword arguments to indicate whether the operator is symmetric and/or | |
| hermitian. The default storage type `S` is determined by `storage_type(M)`, which | |
| typically evaluates to `Vector{T}` for standard matrix types. Change `S` to use | |
| LinearOperators on GPU. |
There was a problem hiding this comment.
This is inherited from previous code.
| return LinearOperator{T, S}(M; kwargs...) | ||
| end | ||
|
|
||
| function LinearOperator{T, S}(M::AbstractMatrix{T}; symmetric = defaultsymmetric(M), hermitian = defaulthermitian(M)) where {T, S} |
There was a problem hiding this comment.
This LinearOperator{T, S}(M::AbstractMatrix{T}; ...) constructor is missing documentation. Since the PR goal is to "Describe LinearOperator{T, S}(args...) in docs" and this is the main matrix constructor that implements the symmetric and hermitian defaults, it should have its own docstring explaining its purpose as the type-inferrable variant and documenting its parameters.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
==========================================
- Coverage 95.00% 94.87% -0.13%
==========================================
Files 17 21 +4
Lines 1100 1171 +71
==========================================
+ Hits 1045 1111 +66
- Misses 55 60 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
amontoison
merged commit b2259b6
into
JuliaSmoothOptimizers:main
Add documentation for #383
Also cleans up an unnecessarily large number of
LinearOperator(M)methods, preferring to consolidate them by supplyingM-specific defaults for thesymmetricandhermitiankeyword arguments.